Skip to content

Conversation

@weathon
Copy link
Collaborator

@weathon weathon commented Mar 21, 2022

No description provided.

##Ignore test output files

*.csv

Copy link
Owner

@ngs333 ngs333 Mar 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR, this file is showing up as a new file, which should not be the case as there is one in the main brach.
I'm going ahead and update the repo ham brach with the main .gitignore. You can merge from either the main
brach or the ham beach on the repo. I am not 100% sure, but I think you can just do a git fetch and git pull.
If you have any issues, we can have a video conference.

@@ -0,0 +1,44 @@
# Install script for directory: /home/wg25r/new/CMT/src/apps
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files cmake_install.cmake is another (like Makefiles) that are auto generated and we dont want to install in
the repo. If you are adding them on purpose, let me know. But in the mean time I am adding them to the .gitignore.

@@ -0,0 +1,8 @@
# CMake generated Testfile for
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not want to change anything in the the vendor subdirs. Can you plase not add files to stage via "add -A" (or the equivslent in the gui). In gour case, you need to manually select which files to add. Please unstage any vendor libs
before committing or just dont stage them.

=======
# WG all locations
**/CMakeFiles
**/.vscode
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The directoryy .vscode under /CMT/src should be tracked.

@@ -0,0 +1,63 @@
# Install script for directory: /home/wg25r/new/CMT/src
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the other *.cmake files, we are not tracking this one in github.

Copy link
Owner

@ngs333 ngs333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weathon

Wayne,

If you look at your own pull request you will see that there are 31 files
the pull request is changing in some way. I am here attaching a png screenshot
of what it looks like. Anything with a + sign in a square is a new file. Anything with a dot in a square a modified file. I have made a few comments in the code, in summary:

  1. we do not want to change or add in directory vendor/edlib
  2. We are not tracking files named Makefile or cmake_install.cmake – these are auto generated.
  3. we are not tracking any .a files (like libedlib.a which shows up in the /lib dir)
  4. There is an empty directory with an empty file – we are not tracking these.

You are probably staging pre-commit these thought a command like “git add -A” or though a GUI
command that is the equivalent. You might want to consider adding your files
for staging one at a t time like:
cd CMT
git add src/metric/HM.h
gut add src/metric/Metric.h
etc

and then committing before pushing.

Of course there are many ways to achieve that above, but in any case you should inspect
your own PR and make sure you are not adding any of these unwanted files.

BTW, directory .vscode should be tracked.

@ngs333
Copy link
Owner

ngs333 commented Apr 3, 2022

@weathon
Attached in the PR view I mentioned in #3 (review).

pull_request_review

Copy link
Owner

@ngs333 ngs333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are numerous issued. See my comments thought the code. BTW, if you want to bring in a new external library into the project. Send me the info of that lib (e.g. its github location or other location on the web) and I will figure out how to bring it in.

Plase note the "using" statement right after the end of the class definition,
which essentially fixes the dimention and data type for the project.
*/
const float randomVector[100] = {1.293421904122479, 1.695729592843372, 1.6719711547501275, 1.1125228837930246, 0.3463672735101978, 2.5971576840006523, 0.47356529987958007, 1.607870549803747, 0.504348570202557, 0.1327015965966274, 0.8407478678805063, 0.6456343347779361, 0.46206423584782796, 2.8044360904022763, 0.3534170421373124, 0.6282496527409083, 1.9274343239417293, 0.4990746743196527, 0.5921481940274671, 0.8130506394674535, 0.9179098654981169, 2.200866777474638, 0.40211321758317176, -1.3506930204269914, 0.12762957633867444, 0.10920202607758023, 2.570930325030065, -0.4605109329252228, 0.8423316879709882, 1.15205339995524, -0.0740485753715252, 0.045156458950561085, 0.2486728341172454, 1.0997250136431282, 1.6454537117301937, 0.16106755903944003, 1.3920690584862614, -1.4652215168295029, -0.5424358569542365, -0.6279762680635221, 0.7619374180187857, 3.0929171613939617, 1.3566763325755058, -0.5312541712966932, -0.02377305717104128, 0.7017996282633281, 0.6932799170379931, 2.404980607473966, -0.008448366109641947, 2.3335701143286656, 1.3090544685544825, 2.191448471203553, 0.9563462046503907, 1.1400149967902473, 0.060111570351233934, -0.44140936723571844, 0.7249121336233216, 0.33947909232267026, 0.4648919733787267, 0.7053070163166842, 1.633584717421872, 1.19772923388379, 0.9386077636155997, 1.104913523982289, 2.4739178942285793, -1.054105970320074, 0.09365996218770678, 1.4459017426807432, -0.24716241373271308, 0.9539362096588104, 2.5927566117627183, 1.331241099010263, 0.5441611494927473, 0.612775214196773, 2.175193539933754, 0.03082934287696737, 3.4984740065183204, 1.324657895747359, 1.3169343164489682, 1.0419990579690943, 0.9771503679842581, 0.9961158732634766, 2.1773811411945507, 1.0589228792469443, 0.7382605888100975, 0.34476012880302054, 2.5047089037430803, 1.9419190323933844, 0.25790869276220685, 1.6906093837845844, 0.3285786883488989, 2.8397878421918317, 0.8987657999056778, 1.8006459208447074, 0.5466175819849189, 1.1089715571944254, 0.9068432426977688, 1.1991397711007348, -0.260606133167711, 0.535703106828273};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Its not a good idea to hard code numbers as its done here.
    If you want to use the same random number every time the program runs, you should learn how to seed a random number generator (possibly with function srand) before making the various calls to function rand to generate the numbers.
  2. The size of the array should not be hard coded as is. It depends on the dimensionality, doesn't it?

// }
};
static const unsigned int HMDIM = 10;
using EuclidianPointPointType = float;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class should use the EuclidianPointPointType.

@@ -0,0 +1,203 @@
# CMAKE generated file: DO NOT EDIT!
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makefiles are auto generated. We are not putting any in the repos.

@@ -0,0 +1,44 @@
# Install script for directory: /home/wg25r/new/CMT/src/metric
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another file that is auto generated and that we are not saving in the repos.

@@ -0,0 +1,120 @@
#ifndef _APPLICATION_H_
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an external library? If and library cannot be brought in
via the git sub-module mechanism, all we can do is mention it in the README and tell users how to get it and where to put it. We just cannot store other peoples library in the repo.

It looks like there are other source files here from gedLib. They all get the same comment as in the paragraph above.

@@ -0,0 +1,1638 @@
#include "Application.h"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an external library? If and library cannot be brought in
via the git sub-module mechanism, all we can do is mention it in the README and tell users how to get it and where to put it. We just cannot store other peoples library in the repo.

@@ -0,0 +1,10 @@
t 1 methane
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a data file? this belongs in a different directory.

@@ -0,0 +1,8 @@
#include "gedLib/ged.h"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apps dont belong in the src/metric direcotry.

@@ -0,0 +1,203 @@
# CMAKE generated file: DO NOT EDIT!
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as for the other Makefiles.

@@ -0,0 +1,44 @@
# Install script for directory: /home/wg25r/new/CMT/src/search
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants